Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to allow header timestamps to be UTC #65

Merged
merged 4 commits into from
Apr 22, 2024

Conversation

fireflycons
Copy link
Contributor

Perhaps I'm missing something, but I could not see how to get the header timestamp to be always UTC instead of system local time.

This PR adds a boolean field to Logger so that UTC may be enabled, and the logic for this in header()

Additionally fix a typo in Logger comments and disable running of syslog tests on Windows.

@phuslu
Copy link
Owner

phuslu commented Apr 22, 2024

thanks! will go through it today!

timeZone var isn't accessed if offest is zero
@fireflycons
Copy link
Contributor Author

Out of interest, what reference did you use to craft the ASM to get the go-id?

@phuslu
Copy link
Owner

phuslu commented Apr 22, 2024

I added comment to goid.s, using pure asm to impalement this function can minimalized the function call overhead in go, I think.

@phuslu
Copy link
Owner

phuslu commented Apr 22, 2024

After consideration I thin it's no problem for adding the "TimeUTC" field.

  1. Is there a better name for it? (If currently no, let us merge it first)
  2. Maybe the code change could be simplify/tidy. (maybe I'm wrong, but it will not be a blocker)

@phuslu
Copy link
Owner

phuslu commented Apr 22, 2024

Let me merge it first, thanks.

@phuslu phuslu merged commit 8102d66 into phuslu:master Apr 22, 2024
@phuslu
Copy link
Owner

phuslu commented Apr 22, 2024

Another option is change TimeUTC bool to TimeLocation *time.Location.

If user give a non-nil value(e.g. Time.UTC), we will call Time.In for the timestamp, and we also need implement a fast path for time.UTC.

@phuslu
Copy link
Owner

phuslu commented Apr 22, 2024

@fireflycons please take a look this approach #66 if you have time, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants